Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ambiguous time #3786

Merged
merged 5 commits into from Nov 16, 2018
Merged

Support ambiguous time #3786

merged 5 commits into from Nov 16, 2018

Conversation

breezewish
Copy link
Member

@breezewish breezewish commented Nov 14, 2018

Signed-off-by: Breezewish breezewish@pingcap.com

What have you changed? (mandatory)

Supports parsing date time which is ambiguous due to DST shifting back.

This PR is a workaround that uses some low level chrono API to bypass chronotope/chrono-tz#23. Once chronotope/chrono-tz#23 is fixed, we should switch back using public and high level APIs.

Resolve #3721

What are the type of the changes? (mandatory)

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

Unit test.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

breezewish and others added 2 commits November 14, 2018 18:20
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breezewish breezewish added priority/critical Priority: Critical sig/coprocessor SIG: Coprocessor labels Nov 15, 2018
@breezewish
Copy link
Member Author

@BusyJay @solotzg @DorianZheng This is an important bug fix required by our new clients. PTAL, thanks!

@breezewish breezewish added priority/high Priority: High and removed priority/critical Priority: Critical labels Nov 15, 2018
.and_then(|t| t.checked_add_signed(Duration::nanoseconds(i64::from(nanos))))
.and_then(|datetime| tz.from_local_datetime(&datetime).earliest())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is earliest chosen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because MySQL choses earliest when there is ambiguous time.

For example, in America/Los_Angeles time zone: After Sunday, 06 November, 2011 01:59:59 AM: Clocks were moved backward to become Sunday, 06 November, 2011 01:00:00 AM. When giving 2011-11-06 01:59:59, it can be interpreted to the following two:

  • The time before transition: 2011-11-06 01:59:59 -7 (timestamp: 1320569999)

  • The time after transition: 2011-11-06 01:59:59 -8 (timestamp: 1320573599)

In MySQL:

mysql> SET time_zone = 'America/Los_Angeles';
Query OK, 0 rows affected (0.03 sec)

mysql> select unix_timestamp('2011-11-06 01:59:59');
+---------------------------------------+
| unix_timestamp('2011-11-06 01:59:59') |
+---------------------------------------+
|                            1320569999 |
+---------------------------------------+
1 row in set (0.01 sec)

mysql> select unix_timestamp('2011-11-06 02:00:00');
+---------------------------------------+
| unix_timestamp('2011-11-06 02:00:00') |
+---------------------------------------+
|                            1320573600 |
+---------------------------------------+
1 row in set (0.00 sec)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So earliest is equal to single when the conversion is unique?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@huachaohuang huachaohuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@breezewish
Copy link
Member Author

/run-integration-compatibility-test

@breezewish breezewish merged commit edc6081 into tikv:master Nov 16, 2018
@breezewish breezewish deleted the _dst_shift_back branch November 16, 2018 11:23
@shenli
Copy link
Member

shenli commented Nov 16, 2018

Please cherry-pick this to the release-2.0 and release-2.1.

@breezewish
Copy link
Member Author

@shenli Got

breezewish added a commit to breezewish/tikv that referenced this pull request Nov 28, 2018
Signed-off-by: Breezewish <breezewish@pingcap.com>
BusyJay pushed a commit that referenced this pull request Nov 29, 2018
Signed-off-by: Breezewish <breezewish@pingcap.com>
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: Breezewish <breezewish@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high Priority: High sig/coprocessor SIG: Coprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tikv does not handle ambiguous time zones correctly
5 participants